Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add an exception for a missing administrator email address #5330

Merged
merged 6 commits into from
Oct 11, 2022
Merged

Add an exception for a missing administrator email address #5330

merged 6 commits into from
Oct 11, 2022

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Oct 1, 2022

Having an administrator email address defined in the system settings is pretty important - otherwise you will not be able to send emails from forms or with the registration module for example:

Swift_RfcComplianceException:
Address in mailbox given [] does not comply with RFC 2822, 3.6.2.

  at vendor\swiftmailer\swiftmailer\lib\classes\Swift\Mime\Headers\MailboxHeader.php:355
  at Swift_Mime_Headers_MailboxHeader->assertValidAddress('')
     (vendor\swiftmailer\swiftmailer\lib\classes\Swift\Mime\Headers\MailboxHeader.php:272)
  at Swift_Mime_Headers_MailboxHeader->normalizeMailboxes(array(''))
     (vendor\swiftmailer\swiftmailer\lib\classes\Swift\Mime\Headers\MailboxHeader.php:117)
  at Swift_Mime_Headers_MailboxHeader->setNameAddresses(array(''))
     (vendor\swiftmailer\swiftmailer\lib\classes\Swift\Mime\Headers\MailboxHeader.php:74)
  at Swift_Mime_Headers_MailboxHeader->setFieldBodyModel(array(''))
     (vendor\swiftmailer\swiftmailer\lib\classes\Swift\Mime\SimpleMimeEntity.php:600)
  at Swift_Mime_SimpleMimeEntity->setHeaderFieldModel('From', array(''))
     (vendor\swiftmailer\swiftmailer\lib\classes\Swift\Mime\SimpleMessage.php:213)
  at Swift_Mime_SimpleMessage->setFrom('')
     (vendor\contao\contao\core-bundle\src\Resources\contao\library\Contao\Email.php:458)

However, during the installation process of Contao it is not actually required to enter an administrator email address - so this might be missed completely and the error will not really tell you that the problem is a missing administrator email address.

This PR adds an exception to the Email class in case the administrator email address is not defined - and a getSystemMessage hook in order to show that the Contao installation is still in a misconfigured state.

This PR also adds a small CSS change so that links in error messages are actually identifiable as such:

image

⬇️

image

@fritzmg fritzmg added the bug label Oct 1, 2022
@fritzmg fritzmg added this to the 4.9 milestone Oct 1, 2022
@fritzmg fritzmg self-assigned this Oct 1, 2022
ausi
ausi previously approved these changes Oct 1, 2022
View reviewed changes
m-vo
m-vo previously approved these changes Oct 3, 2022
View reviewed changes
core-bundle/src/EventListener/AdministratorEmailListener.php Outdated Show resolved Hide resolved
leofeyer
leofeyer requested changes Oct 5, 2022
View reviewed changes
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new feature, so please rebase it onto the 5.x branch and change the milestone accordingly.

@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 5, 2022

This is a new feature, so please rebase it onto the 5.x branch and change the milestone accordingly.

I consider the fact that

  • you cannot send emails when not providing an administrator email address
    (which as pointed out above is never set during the installation process)
  • the error in the front end does not tell you what is wrong

a bug and this PR intends to fix that by telling you what is wrong.

@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 5, 2022 •
edited

@leofeyer or do you only view the new getSystemMessage hook (plus its accompanying CSS change and additional translation label) as a feature? We should at the very least add the exception.

@leofeyer
Copy link
Member

leofeyer commented Oct 6, 2022

Adding just the exception as a bug fix would be fine with me.

@fritzmg fritzmg dismissed stale reviews from m-vo and ausi via 79cdeb5 October 6, 2022 14:35
@fritzmg fritzmg requested a review from leofeyer October 6, 2022 14:36
@fritzmg fritzmg changed the title Add an exception and check for a missing administrator email address Add an exception for a missing administrator email address Oct 7, 2022
@leofeyer leofeyer enabled auto-merge (squash) October 11, 2022 07:42
@leofeyer
Copy link
Member

Thank you @fritzmg.

@leofeyer leofeyer merged commit 0936129 into contao:4.9 Oct 11, 2022
@fritzmg fritzmg deleted the check-admin-email branch October 11, 2022 07:43
@aschempp
Copy link
Member

we should consider adding the back end hint in Contao 5.0/5.x since the Contao Manager will not ask for the admin email (contrary to the install tool).

@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 25, 2022 •
edited

(contrary to the install tool).

The install too also will not ask for the administrator email address.

Other than that I wanted to create a PR for 5.x, yes.

@ausi
Copy link
Member

ausi commented Oct 25, 2022

Just a quick Idea: can we fallback to $_SERVER['SERVER_ADMIN']?
Or is this something only apache has set but nginx hasn’t?

@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 25, 2022

On Hostingwerk for example $_SERVER['SERVER_ADMIN'] is [no address given], so I don't think we can/should use that.

@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 25, 2022

We could fall back to the email address of the first non-disabled admin back end user (order by ID ascending) - but that might be too arbitrary.

@fritzmg fritzmg mentioned this pull request Oct 25, 2022
Merged
@aschempp
Copy link
Member

aschempp commented Oct 27, 2022 •
edited

The install too also will not ask for the administrator email address.

it adds the admin email from the first registered user, which is no longer the case with the Contao Manager.

@fritzmg
Copy link
Contributor Author

fritzmg commented Oct 27, 2022

Huh, never noticed that before 😁. So then we could indeed implement this fallback in Contao 5 at least (may be even 4.9+).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-vo m-vo m-vo left review comments

@ausi ausi ausi approved these changes

@leofeyer leofeyer leofeyer approved these changes

Assignees

@fritzmg fritzmg

Labels
bug
Projects
None yet
Milestone
4.9
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fritzmg @leofeyer @aschempp @ausi @m-vo